Skip to content

cg_llvm: Reduce the visibility of types, modules and using declarations in rustc_codegen_llvm. #137247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 25, 2025
Merged

cg_llvm: Reduce the visibility of types, modules and using declarations in rustc_codegen_llvm. #137247

merged 1 commit into from
Mar 25, 2025

Conversation

dpaoliello
Copy link
Contributor

@dpaoliello dpaoliello commented Feb 18, 2025 •
edited by Zalathar
Loading

Final part of #135502

Reduces the visibility of types, modules and using declarations in the rustc_codegen_llvm to private or pub(crate) where possible, and marks unused fields and enum entries with #[expect(dead_code)].

r? Zalathar

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2025
Zalathar
Zalathar reviewed Feb 21, 2025
View reviewed changes
@Zalathar
Copy link
Contributor

I've been thinking about how to deal with the enums that have more than 1-2 unused variants, especially the ones where almost all variants are unused.

Putting #[expect(dead_code)] on each individual variant is the most precise approach, but it has the massive practical downside of making the enums a nightmare to read/audit, which kind of defeats the point of wanting to keep them around while reducing visibility.

So for those enums, I think it's going to be better to just put #[expect(dead_code)] on the enum itself (instead of individual variants), and live with the fact that this makes it harder to see whether the enum is unused (or which variants are unused).

That's a bit of a shame, but it's still an improvement over the status quo of everything being pub.

@Zalathar
Copy link
Contributor

Specifically, I'm proposing that these enums should have #[expect(dead_code)] on the enum itself, and not on its variants:

  • AttributeKind
  • TypeKind
  • MetadataType
  • Opcode

@dpaoliello
Copy link
Contributor Author

Specifically, I'm proposing that these enums should have #[expect(dead_code)] on the enum itself, and not on its variants:

  • AttributeKind
  • TypeKind
  • MetadataType
  • Opcode

Done

@Zalathar
Copy link
Contributor

Adding reasons for the expect attributes was a great idea, but some of them were slightly inaccurate, so I pushed a fix for that (diff).

@Zalathar
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 22, 2025

📌 Commit a5d4c5c has been approved by Zalathar

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 22, 2025
@matthiaskrgr
Rollup merge of rust-lang#137247 - dpaoliello:cleanllvm, r=Zalathar
f8061c5 
cg_llvm: Reduce the visibility of types, modules and using declarations in `rustc_codegen_llvm`.

Final part of rust-lang#135502

Reduces the visibility of types, modules and using declarations in the `rustc_codegen_llvm` to private or `pub(crate)` where possible, and marks unused fields and enum entries with `#[expect(dead_code)]`.

r? `@Zalathar`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 22, 2025
Closed
@matthiaskrgr
Copy link
Member

@bors r-
#137431 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2025
@Zalathar
Copy link
Contributor

Ah, this collided with some more pub items introduced by #136428.

@Zalathar Zalathar mentioned this pull request Feb 24, 2025
Closed
@dpaoliello
Copy link
Contributor Author

Ah, this collided with some more pub items introduced by #136428.

Updated.

@dpaoliello dpaoliello requested a review from Zalathar February 24, 2025 17:34
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2025
@Zalathar
Copy link
Contributor

Hmm, I think I lost track of this because more conflicts occurred shortly after the last rebase; it seems to need another rebase.

@Zalathar
Copy link
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2025
@dpaoliello
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2025
@Zalathar
Copy link
Contributor

Oh, could you please reapply the expect fixes from #137247 (comment) ? They probably got lost when you rebased from your local branch. (Squashing them into the main commit is fine.)

@dpaoliello
Copy link
Contributor Author

Oh, could you please reapply the expect fixes from #137247 (comment) ? They probably got lost when you rebased from your local branch. (Squashing them into the main commit is fine.)

Sorry about that: fixed.

@Zalathar
Copy link
Contributor

Reapplied a clearer commit name, and fixed one expect reason (diff).

@Zalathar
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 25, 2025

📌 Commit 79b9664 has been approved by Zalathar

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 25, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2025
@bors
Auto merge of rust-lang#138933 - matthiaskrgr:rollup-sjtqkoq, r=matth…
43f0014 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#135745 (Recognise new IPv6 non-global range from IETF RFC 9602)
 - rust-lang#137247 (cg_llvm: Reduce the visibility of types, modules and using declarations in `rustc_codegen_llvm`.)
 - rust-lang#138317 (privacy: Visit types and traits in impls in type privacy lints)
 - rust-lang#138581 (Abort in deadlock handler if we fail to get a query map)
 - rust-lang#138776 (coverage: Separate span-extraction from unexpansion)
 - rust-lang#138886 (Fix autofix for `self` and `self as …` in `unused_imports` lint)
 - rust-lang#138924 (Reduce `kw::Empty` usage, part 3)
 - rust-lang#138929 (Visitors track whether an assoc item is in a trait impl or an inherent impl)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b66e932 into rust-lang:master Mar 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 25, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2025
@rust-timer
Unrolled build for rust-lang#137247
2cf5148 
Rollup merge of rust-lang#137247 - dpaoliello:cleanllvm, r=Zalathar

cg_llvm: Reduce the visibility of types, modules and using declarations in `rustc_codegen_llvm`.

Final part of rust-lang#135502

Reduces the visibility of types, modules and using declarations in the `rustc_codegen_llvm` to private or `pub(crate)` where possible, and marks unused fields and enum entries with `#[expect(dead_code)]`.

r? Zalathar
@dpaoliello dpaoliello deleted the cleanllvm branch April 3, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zalathar Zalathar Awaiting requested review from Zalathar

Assignees

@Zalathar Zalathar

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dpaoliello @Zalathar @bors @matthiaskrgr @rustbot